New mechanism for ---@return_cast #810
Conversation
Summary of ChangesHello @MillhioreBT, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature for ---@return_cast to allow specifying a fallback type for falsy conditions, which is a great addition for more precise type inference in if/else blocks. The implementation looks solid, covering parsing, analysis, and type narrowing logic. The added tests are comprehensive, ensuring the new functionality works as expected and doesn't break backward compatibility.
I've left a couple of comments regarding code duplication. One is a suggestion to refactor a smaller duplicated block using a closure for better readability. The other points out a larger block of duplicated logic that should be extracted into a helper function to improve maintainability. Addressing these will make the code cleaner and easier to manage in the future.
| // Choose the appropriate cast based on condition_flow and whether fallback exists | ||
| let result_type = match condition_flow { | ||
| InferConditionFlow::TrueCondition => { | ||
| let Some(cast_op_type) = signature_cast.cast.to_node(&signature_root) else { | ||
| return Ok(ResultTypeOrContinue::Continue); | ||
| }; | ||
| cast_type( | ||
| db, | ||
| signature_id.get_file_id(), | ||
| cast_op_type, | ||
| antecedent_type, | ||
| condition_flow, | ||
| )? | ||
| } | ||
| InferConditionFlow::FalseCondition => { | ||
| // Use fallback_cast if available, otherwise use the default behavior | ||
| if let Some(fallback_cast_ptr) = &signature_cast.fallback_cast { | ||
| let Some(fallback_op_type) = fallback_cast_ptr.to_node(&signature_root) else { | ||
| return Ok(ResultTypeOrContinue::Continue); | ||
| }; | ||
| cast_type( | ||
| db, | ||
| signature_id.get_file_id(), | ||
| fallback_op_type, | ||
| antecedent_type.clone(), | ||
| InferConditionFlow::TrueCondition, // Apply fallback as force cast | ||
| )? | ||
| } else { | ||
| // Original behavior: remove the true type from antecedent | ||
| let Some(cast_op_type) = signature_cast.cast.to_node(&signature_root) else { | ||
| return Ok(ResultTypeOrContinue::Continue); | ||
| }; | ||
| cast_type( | ||
| db, | ||
| signature_id.get_file_id(), | ||
| cast_op_type, | ||
| antecedent_type, | ||
| condition_flow, | ||
| )? | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
This block of logic for choosing and applying the type cast is duplicated in get_type_at_call_expr_by_signature_param_name (lines 341-382). To improve maintainability and reduce redundancy, you should extract this logic into a new private helper function. This function could take db, signature_id, signature_cast, signature_root, antecedent_type, and condition_flow as arguments.
| // Bind the true condition type | ||
| if let Some(node_type) = cast_op_type.get_type() { | ||
| let typ = infer_type(analyzer, node_type.clone()); | ||
| let infiled_syntax_id = InFiled::new(analyzer.file_id, node_type.get_syntax_id()); | ||
| let type_owner = LuaTypeOwner::SyntaxId(infiled_syntax_id); | ||
| bind_type(analyzer.db, type_owner, LuaTypeCache::DocType(typ)); | ||
| }; | ||
|
|
||
| // Bind the false condition type if present | ||
| let fallback_cast = if op_types.len() > 1 { | ||
| let fallback_op_type = &op_types[1]; | ||
| if let Some(node_type) = fallback_op_type.get_type() { | ||
| let typ = infer_type(analyzer, node_type.clone()); | ||
| let infiled_syntax_id = InFiled::new(analyzer.file_id, node_type.get_syntax_id()); | ||
| let type_owner = LuaTypeOwner::SyntaxId(infiled_syntax_id); | ||
| bind_type(analyzer.db, type_owner, LuaTypeCache::DocType(typ)); | ||
| } | ||
| Some(fallback_op_type.to_ptr()) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
There's some code duplication when binding the types for the true and false conditions. This can be refactored into a local closure to improve readability and maintainability. Using op_types.get(1).map(...) is also more idiomatic than checking length and then indexing.
let bind_op_type = |op_type: &emmylua_parser::LuaDocOpType| {
if let Some(node_type) = op_type.get_type() {
let typ = infer_type(analyzer, node_type.clone());
let infiled_syntax_id = InFiled::new(analyzer.file_id, node_type.get_syntax_id());
let type_owner = LuaTypeOwner::SyntaxId(infiled_syntax_id);
bind_type(analyzer.db, type_owner, LuaTypeCache::DocType(typ));
}
};
// Bind the true condition type
bind_op_type(cast_op_type);
// Bind the false condition type if present
let fallback_cast = op_types.get(1).map(|fallback_op_type| {
bind_op_type(fallback_op_type);
fallback_op_type.to_ptr()
});
|
please fix the clippy lint |
|
@CppCXY Do you think it's worth applying the suggestions that Gemini code assist says? |
2 participants
I propose this new functionality for ---@return_cast
This allows us to configure a type in case of a falsy type
This functionality allows you to express mutually exclusive type relationships, where knowing that something is NOT of one type automatically tells you that it IS of another specific type. Without this, you lose valuable information in the else branch and need to do redundant additional checks. It's especially powerful in domains where you have closed type hierarchies (a finite and known number of subtypes), such as in video games, simulations, or systems with well-defined taxonomies.
While it is not useful in all contexts,
Its use can be omitted if not necessary
Here are some examples with various types of hierarchies:
More Precise Modeling of Complex Type Guards
Elimination of Ambiguity in Unions
Multiple Discrimination Hierarchies
Reduction of Redundant Code
I don't know if this is the best way to write this functionality, if someone wants to write another PR or if you want to suggest changes or improve this one, I'll be happy to support any decision
I wouldn't mind if it's not implemented, but I want to help give ideas to improve this wonderful project ❤